Telemetry store agent tested and dusted - #1

pkg/agent/agent.go (3)

215-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fold buildSnapshot into poll to remove the duplicated snapshot literal.

Both paths build the identical struct and both call GetTopology; having two copies invites drift (e.g. the cancellation path would silently miss any new field).

♻️ Proposed refactor

 	for _, gpuID := range a.cfg.GPUIDs {
 		select {
 		case <-ctx.Done():
-			remaining := len(a.cfg.GPUIDs) - len(gpus)
-			errCount += remaining
+			errCount += len(a.cfg.GPUIDs) - len(gpus)
 			for i := len(gpus); i < len(a.cfg.GPUIDs); i++ {
 				gpus = append(gpus, conservativeGPU(a.cfg.GPUIDs[i]))
 			}
 			return a.buildSnapshot(gpus), errCount
 		default:
 		}
@@
-	// Collect topology (best-effort; errors produce a zero TopologyMatrix).
-	topo, _ := a.provider.GetTopology(a.cfg.NodeID)
-
-	snap := telemetry.NodeTelemetry{
-		NodeID:        a.cfg.NodeID,
-		SchemaVersion: telemetry.SchemaVersion,
-		GPUs:          gpus,
-		Topology:      topo,
-		CollectedAt:   a.cfg.clock(),
-		Stale:         false,
-	}
-	return snap, errCount
+	return a.buildSnapshot(gpus), errCount
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/agent/agent.go` around lines 215 - 268, Fold the cancellation-path
snapshot construction from buildSnapshot into poll so NodeTelemetry is assembled
in one place. Preserve the existing topology lookup, collected timestamp,
fields, and cancellation behavior, then remove the now-unused
Agent.buildSnapshot method.

---

187-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

duration bypasses the injected clock.

start comes from a.cfg.clock() but the elapsed time uses time.Since, so any non-real clock yields a meaningless duration in OnPollComplete. Use the same source for both.

♻️ Proposed fix

 	start := a.cfg.clock()
 	snapshot, errCount := a.poll(ctx)
-	duration := time.Since(start)
+	duration := a.cfg.clock().Sub(start)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/agent/agent.go` around lines 187 - 201, Update runOneTick to calculate
duration using a second reading from the injected a.cfg.clock instead of
time.Since(start), ensuring OnPollComplete receives elapsed time from the same
clock source used for start.

---

344-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Collapse the three retry helpers into one generic helper. The bodies are identical, and this module already targets Go 1.22, so a single free function taking *Agent can replace all three without interface{} or reflection.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/agent/agent.go` around lines 344 - 393, Replace retryFloat, retryInt64,
and retryUint64 with one Go 1.22 generic free function that accepts *Agent and a
typed callback, preserving the existing retry count, delay, success return, and
final error behavior; update all callers to use the generic helper and remove
the duplicated methods.

pkg/store/store.go (1)

125-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Deduplicate the two sweep implementations.

♻️ Proposed refactor

 func (s *TelemetryStore) MarkStale(threshold time.Duration) {
-	now := time.Now()
-	s.mu.Lock()
-	for id, rec := range s.records {
-		if !rec.Stale && rec.IsStaleAt(now, threshold) {
-			rec.Stale = true
-			s.records[id] = rec
-		}
-	}
-	s.mu.Unlock()
+	s.MarkStaleAt(time.Now(), threshold)
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/store/store.go` around lines 125 - 148, Deduplicate the stale-record
sweep by having MarkStale obtain the current time and delegate to MarkStaleAt,
leaving the iteration and mutation logic centralized in MarkStaleAt. Preserve
the existing stale-marking behavior and locking semantics.

pkg/telemetry/model.go (1)

157-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Collapse IsStale into IsStaleAt.

Duplicated staleness logic; keeps the boundary semantics in one place.

♻️ Proposed refactor

 func (n NodeTelemetry) IsStale(threshold time.Duration) bool {
-	if n.CollectedAt.IsZero() {
-		// A zero timestamp is treated as infinitely stale.
-		return true
-	}
-	return time.Since(n.CollectedAt) > threshold
+	return n.IsStaleAt(time.Now(), threshold)
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/telemetry/model.go` around lines 157 - 173, Remove the duplicated
staleness logic from IsStale and make it delegate to IsStaleAt using the current
time. Preserve the existing zero-timestamp and threshold boundary semantics
centralized in IsStaleAt.

pkg/store/store_test.go (1)

36-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for Set's input deep copy.

Reads are well covered, but the write boundary isn't: mutating the caller's record after Set should not affect the store.

💚 Suggested test

func TestStore_Set_DeepCopiesInput(t *testing.T) {
	s := store.NewTelemetryStore()
	rec := telemetry.NodeTelemetry{
		NodeID:        "node-1",
		SchemaVersion: telemetry.SchemaVersion,
		CollectedAt:   time.Now(),
		GPUs:          []telemetry.GPUTelemetry{{GPUID: "gpu-0"}},
		Topology:      telemetry.TopologyMatrix{Matrix: map[string]map[string]int{"gpu-0": {"gpu-1": 1}}},
	}
	s.Set(rec)

	rec.GPUs[0].GPUID = "mutated"
	rec.Topology.Matrix["gpu-0"]["gpu-1"] = 999

	got, _ := s.Get("node-1")
	if got.GPUs[0].GPUID != "gpu-0" || got.Topology.Matrix["gpu-0"]["gpu-1"] != 1 {
		t.Error("store aliases caller-owned data passed to Set")
	}
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/store/store_test.go` around lines 36 - 48, Add a test alongside
TestStore_SetAndGet that constructs a record with nested GPU and topology map
data, calls Set, mutates those caller-owned nested fields afterward, and
verifies Get still returns the original values, confirming Set deep-copies its
input.

scripts/phase3-manual.sh (2)

55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

kubectl version --short is deprecated/removed on newer kubectl clients.

This is already defensively wrapped (2>/dev/null | head -1 || true), so it won't break the script, but on kubectl releases where --short is fully removed it'll just print a blank client-version line instead of useful info.

🩹 Proposed fix

-echo "  kubectl     : $(kubectl version --client --short 2>/dev/null | head -1 || true)"
+echo "  kubectl     : $(kubectl version --client -o json 2>/dev/null | jq -r '.clientVersion.gitVersion' || true)"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/phase3-manual.sh` at line 55, Update the kubectl version command in
the version-reporting echo to use the current supported client-version
invocation without the deprecated --short flag, while preserving the existing
stderr suppression, first-line extraction, and non-failing fallback behavior.

---

17-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

No cleanup on failure — set -e aborts before Section D runs.

Same concern as the PowerShell counterpart: any failing step in Sections A-C exits the script immediately (via set -e), leaving the CRD and sample object applied to the cluster with no cleanup attempted. A trap 'cleanup' EXIT (with cleanup guarded to run kubectl delete ... --ignore-not-found) would make this idempotent-safe on failure.

Also applies to: 67-169

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/phase3-manual.sh` at line 17, Update scripts/phase3-manual.sh to
register an EXIT trap that invokes the existing cleanup logic when Sections A–C
fail under set -e. Ensure cleanup uses guarded kubectl delete operations with
--ignore-not-found so CRDs and sample objects are removed safely without masking
the original failure.

scripts/phase3-manual.ps1 (1)

41-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

No cleanup on failure — a mid-script Assert-ExitCode failure leaves the CRD/object applied.

Any failed step from A1 through C2 calls exit 1 immediately, skipping Section D. On a shared test cluster this leaves gpunodestatuses.gpu.amshithnair.dev and the sample object behind for manual cleanup. Consider wrapping the lifecycle steps in try/finally so cleanup always runs.

♻️ Sketch

+try {
     # SECTION A, B, C steps...
+}
+finally {
+    Write-Section "SECTION D: Cleanup"
+    kubectl delete gpuns node-gpu-01 -n gpu-scheduler --ignore-not-found
+    kubectl delete crd gpunodestatuses.gpu.amshithnair.dev --ignore-not-found
+}

Also applies to: 88-229

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/phase3-manual.ps1` around lines 41 - 48, Ensure the lifecycle steps
from A1 through C2 execute within try/finally control flow so failures from
Assert-ExitCode do not bypass cleanup. Move or invoke the Section D cleanup for
the CRD and sample object from the finally path, while preserving the existing
success/failure reporting and cleanup behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes